Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimise JSON marshalling for sparse histograms #440

Merged
merged 7 commits into from
Feb 2, 2023

Conversation

zenador
Copy link
Contributor

@zenador zenador commented Jan 27, 2023

Follow up to #417 according to suggestion in #417 (comment)

Benchmarking results:

name                                         old time/op    new time/op    delta
JSONMarshallingSamplePairMatrix-16             6.96µs ± 8%    3.44µs ± 4%  -50.52%  (p=0.000 n=20+19)
JSONMarshallingSampleHistogramPairMatrix-16    69.2µs ± 4%    18.9µs ± 4%  -72.65%  (p=0.000 n=20+19)

name                                         old alloc/op   new alloc/op   delta
JSONMarshallingSamplePairMatrix-16             1.63kB ± 0%    0.84kB ± 0%  -48.31%  (p=0.000 n=20+20)
JSONMarshallingSampleHistogramPairMatrix-16    20.3kB ± 0%     4.6kB ± 0%  -77.17%  (p=0.000 n=20+20)

name                                         old allocs/op  new allocs/op  delta
JSONMarshallingSamplePairMatrix-16               74.0 ± 0%      22.0 ± 0%  -70.27%  (p=0.000 n=20+20)
JSONMarshallingSampleHistogramPairMatrix-16       658 ± 0%        22 ± 0%  -96.66%  (p=0.000 n=20+20)

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
@zenador zenador force-pushed the sparsehistograms branch 5 times, most recently from 09084ca to 0ed9457 Compare January 27, 2023 10:46
Copy link
Member

@codesome codesome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! I only have comments about comments and removing a check in benchmarks

model/value_marshal.go Outdated Show resolved Hide resolved
model/value_marshal.go Outdated Show resolved Hide resolved
model/value_marshal.go Outdated Show resolved Hide resolved
model/value_marshal.go Outdated Show resolved Hide resolved
model/value_histogram_test.go Outdated Show resolved Hide resolved
model/value_float_test.go Outdated Show resolved Hide resolved
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
…amPair

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
@zenador
Copy link
Contributor Author

zenador commented Feb 1, 2023

Thanks for the review! Updated benchmark results:

name                                         old time/op    new time/op    delta
JSONMarshallingSamplePairMatrix-16             6.66µs ± 4%    3.36µs ± 1%  -49.58%  (p=0.000 n=20+19)
JSONMarshallingSampleHistogramPairMatrix-16    68.2µs ± 4%    18.2µs ± 1%  -73.27%  (p=0.000 n=19+20)

name                                         old alloc/op   new alloc/op   delta
JSONMarshallingSamplePairMatrix-16             1.63kB ± 0%    0.84kB ± 0%  -48.31%  (p=0.000 n=18+20)
JSONMarshallingSampleHistogramPairMatrix-16    20.3kB ± 0%     4.6kB ± 0%  -77.17%  (p=0.000 n=20+20)

name                                         old allocs/op  new allocs/op  delta
JSONMarshallingSamplePairMatrix-16               74.0 ± 0%      22.0 ± 0%  -70.27%  (p=0.000 n=20+20)
JSONMarshallingSampleHistogramPairMatrix-16       658 ± 0%        22 ± 0%  -96.66%  (p=0.000 n=20+20)

@codesome codesome merged commit f9c1994 into prometheus:main Feb 2, 2023
krajorama added a commit to grafana/mimir that referenced this pull request Feb 20, 2023
At prometheus/common@f9c1994

For prometheus/common#440
Optimise JSON marshalling for sparse histograms and floats

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
krajorama added a commit to grafana/mimir that referenced this pull request Feb 21, 2023
At prometheus/common@f9c1994

For prometheus/common#440
Optimise JSON marshalling for sparse histograms and floats

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
beorn7 added a commit that referenced this pull request Feb 23, 2023
This seemingly undoes #440, but all the json-iterator usage was
actually pulled up into client_golang in
prometheus/client_golang#1225 . Detailed
explanation there. In short, we would like to keep heavy dependencies
like json-iterator out of prometheus/common/model since many importers
of this package aren't even interested in the JSON marshaling.

Signed-off-by: beorn7 <beorn@grafana.com>
beorn7 added a commit that referenced this pull request Feb 28, 2023
This seemingly undoes #440, but all the json-iterator usage was
actually pulled up into client_golang in
prometheus/client_golang#1225 . Detailed
explanation there. In short, we would like to keep heavy dependencies
like json-iterator out of prometheus/common/model since many importers
of this package aren't even interested in the JSON marshaling.

Signed-off-by: beorn7 <beorn@grafana.com>
radek-ryckowski pushed a commit to goldmansachs/common that referenced this pull request May 18, 2023
This seemingly undoes prometheus#440, but all the json-iterator usage was
actually pulled up into client_golang in
prometheus/client_golang#1225 . Detailed
explanation there. In short, we would like to keep heavy dependencies
like json-iterator out of prometheus/common/model since many importers
of this package aren't even interested in the JSON marshaling.

Signed-off-by: beorn7 <beorn@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants